Handle case where ky responses have no body with a getter for a ReadableStream #1224

pcowgill · 2020-01-22T05:20:40Z

Closes https://github.com/ipfs/js-ipfs-http-client/issues/1215

@hugomrdias @alanshaw Let me know what you think! The tests are passing, I added a new test case, and I've run the lint script.

pcowgill · 2020-01-22T05:23:59Z

The basic approach Hugo and I discussed for this PR was to pass the entire ky response to the helper lib that converts the response to an async iterable rather than just passing the Response.body and hoping that part is enough.

achingbrain

Thanks for this PR, it mostly looks good, a few changes needed mentioned in the comments.

src/lib/stream-to-async-iterable.js

achingbrain · 2020-01-22T08:08:30Z

src/add/index.js

@@ -16,21 +16,36 @@ module.exports = configure(({ ky }) => {
    if (options.chunker) searchParams.set('chunker', options.chunker)
    if (options.cidVersion) searchParams.set('cid-version', options.cidVersion)
    if (options.cidBase) searchParams.set('cid-base', options.cidBase)
-    if (options.enableShardingExperiment != null) searchParams.set('enable-sharding-experiment', options.enableShardingExperiment)
+    if (options.enableShardingExperiment != null) {


Were these changes necessary? I personally like curly brackets around if statement bodies, even one liners, but this has made everything inconsistent and is outside the scope of the intended change.

No, they weren't necessary. They happened due to the default code styling settings I have globally in VS Code. If this style is important and gates PR merging, though, we should modify the linter settings to throw a warning or an error with this style. Ideally, we would get aegir lint --fix to automatically resolve such warnings/errors too, which may happen out of the box when we enable the warning. I'll open an issue for this.

Just opened the issue #1225

I've disabled the Prettier and JavaScript Standard Style extensions for this workspace locally in VS Code so that automatic linting changes won't make it into a PR against this repo next time, but I still think the linting settings change is worth making to make it simpler for others to make PRs in the future.

src/dht/put.js

pcowgill · 2020-01-22T15:15:38Z

Thanks for this PR, it mostly looks good, a few changes needed mentioned in the comments.

@achingbrain Thanks for the quick review! I'll work on addressing the comments now.

pcowgill · 2020-01-22T16:32:43Z

src/lib/stream-to-async-iterable.js

+  // An env where res.body getter for ReadableStream with getReader
+  // is not supported, for example in React Native
+  if (!body) {
+    if (res.arrayBuffer) {


@achingbrain @hugomrdias Do we want to console.log anything in this case, like "falling back to a hackier approach since the fetch being used here doesn't implement the streams spec"?

src/add-from-url.js

src/add/index.js

pcowgill · 2020-01-22T16:52:09Z

src/add/index.js

@@ -52,7 +52,7 @@ module.exports = configure(({ ky }) => {
  }
 })

-function toCoreInterface ({ name, hash, size, mode, mtime }) {
+function toCoreInterface({ name, hash, size, mode, mtime }) {


@achingbrain The aegir linter also could use a rule for this

pcowgill · 2020-01-22T17:12:51Z

@achingbrain Okay, I think I've addressed all of your comments! Thanks again for the quick review. Could you please give this another review when you have a moment?

I reverted all of the style changes manually, although it retrospect it may have been faster to configure some local workspace settings to match the styling conventions of this project. Longer term I think this would be a nice improvement to make.

pcowgill · 2020-01-24T17:52:19Z

Yikes, it looks like there are a lot of merge conflicts now that another PR was merged. I'll work on resolving them.

pcowgill · 2020-01-24T17:57:38Z

Looks like it was this PR #1183

pcowgill · 2020-01-26T20:44:10Z

@achingbrain @hugomrdias I don't think getting the conflicts resolved gates an interim PR review, though, in case you have time. Thanks!

pcowgill · 2020-01-28T22:40:28Z

@achingbrain I fixed the merge conflicts. We need to decide how we want to handle this issue - alanshaw/stream-to-it#3 (comment) - before merging this, now that one of the previously modified files, lib/stream-to-async-iterable, was extracted into this shared package

pcowgill · 2020-01-29T04:22:10Z

Some tests are failing with the new logic introduced in this PR, like this one:

  1) .files (the MFS API part)
       .add pins by default:
     Error: Neither Response.body nor Response.arrayBuffer is defined

Interestingly those tests weren't failing with the earlier version of these changes against version 41.x.

pcowgill · 2020-01-29T16:56:50Z

The test failure was because I did a find/replace relative to an earlier version master, so the changes were missing some new uses of res.body that needed to be changed to res in the latest version of master. The tests should be passing now 🤞

pcowgill · 2020-01-29T17:40:43Z

It looks like the Travis failure is just it hitting a timeout, which I don't think is something that my PR would increase the probability of, perhaps something that's just stochastic?

pcowgill · 2020-01-29T18:04:33Z

As for the codecov warnings, it looks like codecov never ran for #1183, so the difference may be from that PR

alanshaw · 2020-01-29T21:26:17Z

@pcowgill apologies I didn't get to this sooner. Did you look into adding a global afterResponse hook when in react native?

It might be less invasive to add a .body property to the response here with an async iterable that calls .arrayBuffer(). Then all the rest of the code can remain the same and it would be easier to remove when RN supports .body.

What do you think?

pcowgill · 2020-01-29T21:32:46Z

@alanshaw To make sure I understand the suggestion, which file are you thinking that change would land in? Thanks!

pcowgill · 2020-01-29T21:33:54Z

@alanshaw Curious for your thoughts on the codecov comment too!

alanshaw · 2020-01-30T12:13:42Z

@alanshaw To make sure I understand the suggestion, which file are you thinking that change would land in? Thanks!

https://github.com/ipfs/js-ipfs-http-client/blob/e9aaa750beeb000f485fb31d43d2af648676dc8e/src/lib/configure.js#L40-L42

elmariachi111 · 2020-02-07T00:22:36Z

Just checked out this PR because when I run jest on my setup (ionic / Typescript / React / node 12), and just test a simple ipfs.cat it fails with Cannot read property 'destroy' of undefined. This PR's fix works 99% around that issue by supporting res.arrayBuffer instead of res.body. But I think another cause / usage is hidden in error-handler#21 -> this line fails first on my machine. When I hack into it with

if ((isNode || isElectronMain) && response.body) response.body.destroy()

my jest tests are green again :)

hugomrdias · 2020-02-07T08:25:13Z

we will be doing some changes around http requests handling soon to fix these issues, stay tuned.

pcowgill · 2020-02-20T17:45:10Z

we will be doing some changes around http requests handling soon to fix these issues, stay tuned.

@hugomrdias That's great! Can you share any additional details about this? Thanks!

pcowgill · 2020-03-09T18:09:36Z

@pcowgill apologies I didn't get to this sooner. Did you look into adding a global afterResponse hook when in react native?

It might be less invasive to add a .body property to the response here with an async iterable that calls .arrayBuffer(). Then all the rest of the code can remain the same and it would be easier to remove when RN supports .body.

What do you think?

@alanshaw Sure, that suggestion works for me.

@hugomrdias Are you already planning on implementing the changes Alan described in the child package of the js-ipfs monorepo as part of the ky removal, or should I make a PR against the ky removal branch with these changes? (Assuming the ky removal branch will take at least a few more days to be merged.)

pcowgill · 2020-03-09T18:10:27Z

Will there still be the equivalent of an afterResponse hook after removing ky?

hugomrdias · 2020-03-09T18:33:47Z

There will be no hooks and would advise against making the PR that specific problem needs to be fixed outside http-client

pcowgill added 2 commits January 21, 2020 21:45

Handling case where res.body is undefined

f0d4016

Fix tests, add test, rename to async iterable, fix addFromURL

37b7d27

pcowgill changed the title ~~Handle no response body with getter for ReadableStream case in ky requests~~ Handle case where ky requests have no response body with a getter for a ReadableStream Jan 22, 2020

pcowgill changed the title ~~Handle case where ky requests have no response body with a getter for a ReadableStream~~ Handle case where ky responses have no body with a getter for a ReadableStream Jan 22, 2020

pcowgill requested a review from hugomrdias January 22, 2020 05:21

achingbrain suggested changes Jan 22, 2020

View reviewed changes

pcowgill mentioned this pull request Jan 22, 2020

Linter should enforce coding style rules that will gate merging PRs #1225

Closed

pcowgill added 2 commits January 22, 2020 10:24

Added local editor config dir to .gitignore

3ee0016

Handling case where body and arrayBuffer both are not defined

74c286e

pcowgill commented Jan 22, 2020

View reviewed changes

Reverting whitespace changes

528a0b9

pcowgill commented Jan 22, 2020

View reviewed changes

src/add-from-url.js Outdated Show resolved Hide resolved

pcowgill commented Jan 22, 2020

View reviewed changes

src/add/index.js Outdated Show resolved Hide resolved

pcowgill commented Jan 22, 2020

View reviewed changes

pcowgill added 3 commits January 22, 2020 10:58

Reverting more automatic style changes

4ea0b03

Reverting paren change for single param arrow function

02b6269

Reverting generator function style change

4819f0e

pcowgill requested a review from achingbrain January 22, 2020 17:09

Fixed linting errors

7a0d504

pcowgill mentioned this pull request Jan 28, 2020

Handle case where body is undefined in React Native env alanshaw/stream-to-it#3

Closed

Merge branch 'master' into feature/handle-no-response-body

5fffc22

pcowgill added 2 commits January 28, 2020 16:49

Augmenting the behavior of stream-to-it/source

2508489

Fixed relative imports

6585971

Fixed test failures

843a97e

pcowgill mentioned this pull request Feb 20, 2020

Linter/formatter should enforce coding style rules that will gate merging PRs ipfs/aegir#504

Closed

This was referenced Mar 2, 2020

Tracking issue for ky removal ipfs/js-ipfs#2801

Closed

Track: react native support ipfs/js-ipfs#2813

Closed

This was referenced Mar 9, 2020

Handle cases where http responses have no body with a getter for a ReadableStream ipfs/js-ipfs#2871

Closed

WIP: Modify response to add body in React Native and logging daemon requests ipfs/js-ipfs#2874

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle case where ky responses have no body with a getter for a ReadableStream #1224

Handle case where ky responses have no body with a getter for a ReadableStream #1224

pcowgill commented Jan 22, 2020

pcowgill commented Jan 22, 2020

achingbrain left a comment

achingbrain Jan 22, 2020

pcowgill Jan 22, 2020

pcowgill Jan 22, 2020

pcowgill Jan 22, 2020

pcowgill commented Jan 22, 2020

pcowgill Jan 22, 2020 •

edited

Loading

pcowgill Jan 22, 2020 •

edited

Loading

pcowgill commented Jan 22, 2020

pcowgill commented Jan 24, 2020

pcowgill commented Jan 24, 2020

pcowgill commented Jan 26, 2020

pcowgill commented Jan 28, 2020

pcowgill commented Jan 29, 2020

pcowgill commented Jan 29, 2020

pcowgill commented Jan 29, 2020

pcowgill commented Jan 29, 2020

alanshaw commented Jan 29, 2020 •

edited

Loading

pcowgill commented Jan 29, 2020

pcowgill commented Jan 29, 2020

alanshaw commented Jan 30, 2020

elmariachi111 commented Feb 7, 2020

hugomrdias commented Feb 7, 2020

pcowgill commented Feb 20, 2020

pcowgill commented Mar 9, 2020

pcowgill commented Mar 9, 2020 •

edited

Loading

hugomrdias commented Mar 9, 2020

Handle case where ky responses have no body with a getter for a ReadableStream #1224

Are you sure you want to change the base?

Handle case where ky responses have no body with a getter for a ReadableStream #1224

Conversation

pcowgill commented Jan 22, 2020

pcowgill commented Jan 22, 2020

achingbrain left a comment

Choose a reason for hiding this comment

achingbrain Jan 22, 2020

Choose a reason for hiding this comment

pcowgill Jan 22, 2020

Choose a reason for hiding this comment

pcowgill Jan 22, 2020

Choose a reason for hiding this comment

pcowgill Jan 22, 2020

Choose a reason for hiding this comment

pcowgill commented Jan 22, 2020

pcowgill Jan 22, 2020 • edited Loading

Choose a reason for hiding this comment

pcowgill Jan 22, 2020 • edited Loading

Choose a reason for hiding this comment

pcowgill commented Jan 22, 2020

pcowgill commented Jan 24, 2020

pcowgill commented Jan 24, 2020

pcowgill commented Jan 26, 2020

pcowgill commented Jan 28, 2020

pcowgill commented Jan 29, 2020

pcowgill commented Jan 29, 2020

pcowgill commented Jan 29, 2020

pcowgill commented Jan 29, 2020

alanshaw commented Jan 29, 2020 • edited Loading

pcowgill commented Jan 29, 2020

pcowgill commented Jan 29, 2020

alanshaw commented Jan 30, 2020

elmariachi111 commented Feb 7, 2020

hugomrdias commented Feb 7, 2020

pcowgill commented Feb 20, 2020

pcowgill commented Mar 9, 2020

pcowgill commented Mar 9, 2020 • edited Loading

hugomrdias commented Mar 9, 2020

pcowgill Jan 22, 2020 •

edited

Loading

pcowgill Jan 22, 2020 •

edited

Loading

alanshaw commented Jan 29, 2020 •

edited

Loading

pcowgill commented Mar 9, 2020 •

edited

Loading